content: Use RealmStore.serverThumbnailFormats for thumbnails #1987

chrisbobbe · 2025-11-13T02:30:04Z

Fixes #1936.

rajveermalviya

Thanks @chrisbobbe! LGTM, just one small comment.

rajveermalviya · 2025-11-17T12:43:44Z

test/model/content_test.dart

-          --data-urlencode 'narrow=[{"operator":"sender", "operand":2187},
+          --data-urlencode 'narrow=[{"operator":"sender", "operand":13313},
                                    {"operator":"stream", "operand":"test here"},
-                                    {"operator":"topic",  "operand":"content"}]' \
+                                    {"operator":"topic",  "operand":"Thumbnails"}]' \


Left from debug?

This might've been done more simply by just adding the boolean value as a new field like `thumbnailAnimated` on ImagePreviewNode. But this value works closely with some structured data in thumbnail URL paths, here in image previews and also in the upcoming "inline images" feature, zulip#1913. So this new ImageThumbnailLocator class seemed like a helpful way to present the data to consumers.

Fixes zulip#1936.

gnprice

Thanks for taking care of this! Comments below.

gnprice · 2025-11-22T03:11:47Z

lib/model/content.dart

+  /// Use [ImageThumbnailLocatorExtension.resolve] to obtain a suitable URL
+  /// for the current UI need.
+  /// It may not work without adding authentication credentials to the request.


This seems like information for the ImageThumbnailLocator doc.

gnprice · 2025-11-22T03:14:54Z

lib/model/content.dart

+  }
+
+  @override
+  int get hashCode => Object.hash(urlPath, hasAnimatedVersion);


In hashCode, seed the hash with the class name so that the hash doesn't collide with other objects that happen to also consist of a string and a bool:

Suggested change

int get hashCode => Object.hash(urlPath, hasAnimatedVersion);

int get hashCode => Object.hash('ImageThumbnailLocator', urlPath, hasAnimatedVersion);

gnprice · 2025-11-22T03:19:26Z

test/model/content_test.dart

+        thumbnail: ImageThumbnailLocator(
+          urlPath: '/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp',
+          hasAnimatedVersion: false,
+        ),


nit: can keep these a bit more compact:

Suggested change

thumbnail: ImageThumbnailLocator(

urlPath: '/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp',

hasAnimatedVersion: false,

),

thumbnail: ImageThumbnailLocator(hasAnimatedVersion: false,

urlPath: '/user_uploads/thumbnail/2/ce/nvoNL2LaZOciwGZ-FYagddtK/image.jpg/840x560.webp'),

gnprice · 2025-11-22T03:21:42Z

test/model/content_test.dart

+          urlPath: '/user_uploads/thumbnail/2/9f/tZ9c5ZmsI_cSDZ6ZdJmW8pt4/2c8d985d.gif/840x560-anim.webp',
+          hasAnimatedVersion: true,


Hmm. From this URL path, it sounds like the given path is the animated version. That makes the name "has animated version" seem a bit misleading.

The parser shows the attribute name in the HTML is data-animated. The most direct translation of that would be to call this field just animated. How about we use that name?

gnprice · 2025-11-22T03:25:05Z

lib/model/content.dart

-  final String? thumbnailUrl;
+  final ImageThumbnailLocator? thumbnail;


I think it'd be helpful to split this commit:

An NFC prep commit makes the change on this line, and the other changes that makes necessary, with the new class having only urlPath.

Then a separate commit adds hasAnimatedVersion (or animated per another comment).

There are a lot of places that change in boring ways just from moving this URL path out to the new class, so it'd make the substantive changes easier to read if they were in a separate commit from that.

gnprice · 2025-11-22T03:41:20Z

lib/widgets/image.dart

+    final store = PerAccountStoreWidget.of(context);
+    ThumbnailFormat? bestCandidate;
+
+    final animateIfSupported = animationMode.resolve(context);
+    if (hasAnimatedVersion && animateIfSupported) {
+      bestCandidate ??= _bestFormatOf(store.sortedAnimatedThumbnailFormats,
+                          width: widthPhysicalPx, height: heightPhysicalPx);
+    }
+
+    bestCandidate ??= _bestFormatOf(store.sortedStillThumbnailFormats,
+                        width: widthPhysicalPx, height: heightPhysicalPx);


nit: join up the bestCandidate computation as one stanza (and separate it from getting store)

gnprice · 2025-11-22T03:45:41Z

lib/widgets/image.dart

+    if (bestCandidate == null) {
+      // Odd if we'd need to fall back to the format encoded in [locator]'s path.
+      // Seems theoretically possible though:
+      // maybe this format isn't used now, for new uploads,
+      // but it was used in the past, including for this image.
+      return store.realmUrl.replace(path: urlPath);


This is normal if there are no thumbnail formats at all, right?

In fact I think if this happens, it must be the case that there are no still thumbnail formats.

gnprice · 2025-11-22T03:54:37Z

lib/widgets/image.dart

+      return store.realmUrl.replace(path: urlPath);
+    }
+
+    final lastSlashIndex = urlPath.lastIndexOf('/');


These lines both assume that the URL string in urlPath is indeed just a path, and doesn't also contain a query and/or fragment.

I think it'd be reasonable for the API to guarantee that, but it doesn't appear to now (at https://zulip.com/api/message-formatting#images ). Without such a guarantee, a query component (though maybe not a fragment) would be a reasonable choice for this bit of API in isolation — e.g. Gravatar does that, as seen in our GravatarUrl.

(OTOH we do know it starts with a path component, because it starts with /u.)

So it'd be good to discuss that in #api documentation or #api design. Also to have the ImageThumbnailLocator constructor assert it, like it already does for the start of the path. A suitable check would be that there is no ? or # character.

gnprice · 2025-11-22T04:01:38Z

test/widgets/image_test.dart

+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/1000x2000.png');
+    });
+
+    testWidgets('animated version does not exist', (tester) async {


nit: probably clearest to put this test before its sibling — this seems like the simpler case to think about, and covers most of the interesting logic (and is surely the most common case)

Then the other test can focus just on what changes when hasAnimatedVersion is true.

gnprice · 2025-11-22T04:08:14Z

test/widgets/image_test.dart

+      doCheck(tester, 250, 450, false, hasAnimatedVersion: false,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/500x900.jpg');
+      doCheck(tester, 300, 250, true, hasAnimatedVersion: false,
+        'https://chat.example/user_uploads/thumbnail/1/2/a/pic.jpg/840x560.webp');


Hmm, why do these get different results? Both formats should be big enough for both these desired sizes, right?

… Ah, I see: it's because they're in physical pixels.

Would be good to have a comment highlighting that. 🙂 That's also one of the key points this test is making, so it's good to make it explicit.

chrisbobbe requested a review from rajveermalviya November 13, 2025 02:30

chrisbobbe assigned rajveermalviya Nov 13, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Nov 13, 2025

rajveermalviya approved these changes Nov 17, 2025

View reviewed changes

rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Nov 17, 2025

rajveermalviya assigned gnprice and unassigned rajveermalviya Nov 17, 2025

rajveermalviya requested a review from gnprice November 17, 2025 13:11

chrisbobbe added 4 commits November 18, 2025 11:47

api: Add serverThumbnailFormats to initial snapshot

aff2e37

store: Add RealmStore.serverThumbnailFormats

75f1553

content: Use RealmStore.serverThumbnailFormats for thumbnails

7d51241

Fixes zulip#1936.

chrisbobbe force-pushed the pr-thumbnail-formats branch from b94e0fb to 7d51241 Compare November 18, 2025 20:08

gnprice reviewed Nov 22, 2025

View reviewed changes

	int get hashCode => Object.hash(urlPath, hasAnimatedVersion);
	int get hashCode => Object.hash('ImageThumbnailLocator', urlPath, hasAnimatedVersion);

		urlPath: '/user_uploads/thumbnail/2/9f/tZ9c5ZmsI_cSDZ6ZdJmW8pt4/2c8d985d.gif/840x560-anim.webp',
		hasAnimatedVersion: true,

		final String? thumbnailUrl;
		final ImageThumbnailLocator? thumbnail;

content: Use RealmStore.serverThumbnailFormats for thumbnails #1987

Are you sure you want to change the base?

content: Use RealmStore.serverThumbnailFormats for thumbnails #1987

Uh oh!

Conversation

chrisbobbe commented Nov 13, 2025

Uh oh!

rajveermalviya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants